Conversation
|
✅ Trivy gate: no Critical/High vulnerabilities. Trivy Image Scan SummaryImage: 900119715266.dkr.ecr.eu-west-2.amazonaws.com/whoami:feature-gpcapim-258-sds-access-module
Findings (top 50)
|
|
✅ Trivy gate: no Critical/High issues. Trivy IaC (Terraform) Summary
Findings (top 50)
|
| # URLs for different SDS environments | ||
| SANDBOX_URL = "https://sandbox.api.service.nhs.uk/spine-directory/FHIR/R4" | ||
| INT_URL = "https://int.api.service.nhs.uk/spine-directory/FHIR/R4" | ||
| DEP_UAT_URL = "https://dep.api.service.nhs.uk/spine-directory/FHIR/R4" | ||
| PROD_URL = "https://api.service.nhs.uk/spine-directory/FHIR/R4" |
There was a problem hiding this comment.
I can see that these might be useful long-term, but are they adding value at this point?
There was a problem hiding this comment.
Is there any benefit to removing them? There's no test overhead or anything, and it means as and when later on we want to add them in we don't have to go looking for the addresses or make code changes to be able to handle them.
There was a problem hiding this comment.
(And we'll want at least sandbox and int retained anyway, won't we?)
There was a problem hiding this comment.
Agreed - happy to leave for now.
There was a problem hiding this comment.
Presumably the urls will be passed in as config/env variables. We won't ever handle this class attributes.
Vox-Ben
force-pushed
the
feature/GPCAPIM-258_SDS_access_module
branch
from
11f8a33 to
db8aa1c
Compare
DWolfsNHS
left a comment
DWolfsNHS
left a comment
There was a problem hiding this comment.
Approving for code review, given that this can't merge until the pipeline passes which is pending @neil-sproston implementing a terraform fix.
|
|
Deployment Complete
|
| STUB_SDS: "1" | ||
| STUB_PDS: "1" | ||
| STUB_PROVIDER: "1" |
There was a problem hiding this comment.
Make the variable value meaningful to the context.
| STUB_SDS: "1" | |
| STUB_PDS: "1" | |
| STUB_PROVIDER: "1" | |
| STUB_SDS: "true" | |
| STUB_PDS: "true" | |
| STUB_PROVIDER: "true" |
| # Access record structured interaction ID from | ||
| # https://developer.nhs.uk/apis/gpconnect/accessrecord_structured_development.html#spine-interactions | ||
| ACCESS_RECORD_STRUCTURED_INTERACTION_ID = ( | ||
| "urn:nhs:names:services:gpconnect:fhir:operation:gpc.getstructuredrecord-1" | ||
| ) | ||
|
|
There was a problem hiding this comment.
IMO this didn't need to go into a common class. It is intrinsically linked to the GetStructuredRecordRequest.
Yes it is going to be use in multiple places, and thus it is a common piece of code, but by placing it as it's own constant in the common module, it has lost its tight coupling with the request to which it is related.
| # SDS: Get provider details (ASID + endpoint) for provider ODS | ||
| sds = SdsClient( | ||
| auth_token=auth_token, | ||
| api_key=auth_token, |
There was a problem hiding this comment.
The API key is not the auth token.
There was a problem hiding this comment.
I think leaving auth journey out the client at the moment makes the most sense. In that way, once we have set up our own mock.
| endpoint_url: str | None = None | ||
| if get_endpoint: | ||
| endpoint_bundle = self._query_sds( | ||
| ods_code=ods_code, | ||
| party_key=party_key, | ||
| correlation_id=correlation_id, | ||
| timeout=timeout, | ||
| querytype=self.ENDPOINT, | ||
| ) | ||
| endpoint = self._extract_first_entry(endpoint_bundle) | ||
| if endpoint: | ||
| address = endpoint.get("address") | ||
| if address: | ||
| endpoint_url = str(address).strip() | ||
|
|
||
| return SdsSearchResults(asid=asid, endpoint=endpoint_url) |
There was a problem hiding this comment.
Use guard clauses to reduce the three layers of nested if statements.
| endpoint_url: str | None = None | |
| if get_endpoint: | |
| endpoint_bundle = self._query_sds( | |
| ods_code=ods_code, | |
| party_key=party_key, | |
| correlation_id=correlation_id, | |
| timeout=timeout, | |
| querytype=self.ENDPOINT, | |
| ) | |
| endpoint = self._extract_first_entry(endpoint_bundle) | |
| if endpoint: | |
| address = endpoint.get("address") | |
| if address: | |
| endpoint_url = str(address).strip() | |
| return SdsSearchResults(asid=asid, endpoint=endpoint_url) | |
| endpoint_url: str | None = None | |
| if not get_endpoint: | |
| return SdsSearchResults(asid=asid, endpoint=endpoint_url) | |
| endpoint_bundle = self._query_sds( | |
| ods_code=ods_code, | |
| party_key=party_key, | |
| correlation_id=correlation_id, | |
| timeout=timeout, | |
| querytype=self.ENDPOINT, | |
| ) | |
| endpoint = self._extract_first_entry(endpoint_bundle) | |
| if endpoint and address in endpoint: | |
| address = endpoint.get("address") | |
| endpoint_url = str(address).strip() | |
| return SdsSearchResults(asid=asid, endpoint=endpoint_url) | |
| # URLs for different SDS environments | ||
| SANDBOX_URL = "https://sandbox.api.service.nhs.uk/spine-directory/FHIR/R4" | ||
| INT_URL = "https://int.api.service.nhs.uk/spine-directory/FHIR/R4" | ||
| DEP_UAT_URL = "https://dep.api.service.nhs.uk/spine-directory/FHIR/R4" | ||
| PROD_URL = "https://api.service.nhs.uk/spine-directory/FHIR/R4" |
There was a problem hiding this comment.
Presumably the urls will be passed in as config/env variables. We won't ever handle this class attributes.
| assert result is not None | ||
| assert isinstance(result, SdsSearchResults) | ||
| assert result.asid is not None | ||
| assert result.asid == "asid_PROV" | ||
| assert len(result.asid) > 0 |
There was a problem hiding this comment.
To reduce testing implementation we could remove the second assertion.
And the first, third and final assertions are made redundant by the fourth assertion.
| assert result is not None | |
| assert isinstance(result, SdsSearchResults) | |
| assert result.asid is not None | |
| assert result.asid == "asid_PROV" | |
| assert len(result.asid) > 0 | |
| assert result.asid == "asid_PROV" |
There was a problem hiding this comment.
I find Copilot to be over enthusiastic with assertions.
| class TestSdsIntegration: | ||
| """Integration tests for SDS search operations.""" |
There was a problem hiding this comment.
I'm not sure I would class these as integration tests. Maybe I don't see the stubs as a system with which to integrate.
I would read integration tests as requiring a system to be built/stood up with other built/stood-up systems.
| assert result is not None | ||
| assert result.asid == "asid_PROV" | ||
| assert result.endpoint is not None | ||
| assert result.endpoint == "https://provider.example.com/fhir" | ||
| # Verify endpoint is a valid URL format | ||
| assert result.endpoint.startswith("https://") | ||
| assert "fhir" in result.endpoint |
There was a problem hiding this comment.
Similar to above. This tests also runs the same setup and assertion as the first.
| # Configure external services to use stubs | ||
| # To disable, comment these out - if they're set at all, it'll use the stub | ||
| ENV STUB_SDS=1 | ||
| ENV STUB_PDS=1 | ||
| ENV STUB_PROVIDER=1 |
There was a problem hiding this comment.
I don't think these are necessary.
| ENV STUB_SDS=1 | ||
| ENV STUB_PDS=1 | ||
| ENV STUB_PROVIDER=1 |
4 participants
Description
Creates the SDS access module and integrates it with the controller.
Context
CDG requires the ability to obtain organisation ASIDs and endpoints from SDS. This provides that capability.
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.